Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes for DeepDoubleX #39184

Merged
merged 3 commits into from
Sep 5, 2022
Merged

Fixes for DeepDoubleX #39184

merged 3 commits into from
Sep 5, 2022

Conversation

kpedro88
Copy link
Contributor

PR description:

This PR cleans up a few issues from #30016:

  • removes an unnecessary edm::Ptr that caused an exception when running on packedCandidate: PFJet constituent is not of PFCandidate type (reported on Mattermost)
  • the new dependency on pat::Jet in the DeepDoubleX code was not handled properly: when running on reco::Jet, the dynamic_cast to pat::Jet would fail, creating a nullptr, which would then be passed to commonCandidateToFeatures()/getDRSubjetFeatures() for another dynamic_cast, leading to a segfault.
  • the requirement to run only on pat::Jet should be reflected in jetTools as it is for other discriminators. I added a simple check; not sure if it's optimal or comprehensive in this case.

PR validation:

Ran jetToolbox several times and observed correct behavior (either runs to completion or throws "Input is not a pat::Jet." when expected), rather than unexpected exceptions or segfaults.

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

This PR will be backported to 10_6_X for those running on UL samples. Other backports can be made upon request.

attn: @andrzejnovak

@cmsbuild cmsbuild added this to the CMSSW_12_5_X milestone Aug 25, 2022
@kpedro88 kpedro88 mentioned this pull request Aug 25, 2022
Merged
@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39184/31780

  • This PR adds an extra 36KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @kpedro88 (Kevin Pedro) for master.

It involves the following packages:

  • PhysicsTools/PatAlgos (reconstruction)
  • RecoBTag/FeatureTools (reconstruction)

@jpata, @cmsbuild, @mandrenguyen, @clacaputo can you please review it and eventually sign? Thanks.
@AlexDeMoor, @rappoccio, @gouskos, @jdolen, @ahinzmann, @schoef, @emilbols, @jdamgov, @JyothsnaKomaragiri, @nhanvtran, @gpetruc, @azotz, @hqucms, @hatakeyamak, @mbluj, @gkasieczka, @mariadalfonso, @demuller, @andrzejnovak, @seemasharmafnal, @mmarionncern this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@kpedro88
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e63739/27068/summary.html
COMMIT: cb47dd0
CMSSW: CMSSW_12_5_X_2022-08-24-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39184/27068/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 51
  • DQMHistoTests: Total histograms compared: 3693084
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3693060
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
  • Checked 212 log files, 49 edm output root files, 51 DQM output files
  • TriggerResults: no differences found

@mariadalfonso
Copy link
Contributor

mariadalfonso commented Aug 25, 2022 •
edited
Loading

@kpedro88
do you think would be useful to have in 12-4 for Run3 analysis ? if Yes also backport there

@kpedro88 kpedro88 mentioned this pull request Aug 25, 2022
Merged
@jpata
Copy link
Contributor

jpata commented Aug 26, 2022

@cms-sw/btv-pog-l2 @emilbols just to check you are aware and agree.

@smuzaffar smuzaffar modified the milestones: CMSSW_12_5_X, CMSSW_12_6_X Aug 28, 2022
@soureek
Copy link

soureek commented Sep 1, 2022

@jpata +1 from BTV

@clacaputo
Copy link
Contributor

+reconstruction

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 5, 2022

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@perrotta
Copy link
Contributor

perrotta commented Sep 5, 2022

+1

  • Tests are 11 days old, but still available
  • The fixes are conceptually simple, I see no need to rerun the tests to refresh them

@cmsbuild cmsbuild merged commit 68f3144 into cms-sw:master Sep 5, 2022
This was referenced Sep 5, 2022
Merged
Merged
Merged
@kpedro88 kpedro88 mentioned this pull request Sep 7, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
code-checks-approved fully-signed orp-approved reconstruction-approved tests-approved
Projects
None yet
Milestone
CMSSW_12_6_X
Development

Successfully merging this pull request may close these issues.
8 participants
@kpedro88 @cmsbuild @mariadalfonso @jpata @soureek @clacaputo @perrotta @smuzaffar